✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	ad9f130
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/679f5f4ffc7c670008fcd7b7
😎 Deploy Preview	https://deploy-preview-1688--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

I added the do-not-merge/work-in-progress label for two reasons because maintainers should discuss whether this is a change we want to make and if there is a deeper problem we still want to solve around recovering from interruptions.

Codecov Report

Attention: Patch coverage is 90.47619% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.21%. Comparing base (9b08aea) to head (ad9f130).
Report is 83 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   67.74%   68.21%   +0.46%     
==========================================
  Files          57       58       +1     
  Lines        4620     4697      +77     
==========================================
+ Hits         3130     3204      +74     
- Misses       1265     1268       +3     
  Partials      225      225              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm not sure about this as I worry that using those delayed contexts might make it more difficult to debug and understand things in the future with the additional waiting times.

Maybe alternatively we could try and solve this by adding (additional) cleanup logic when we get the parent context cancellation for the components that might need it and then placing single-place 'global' timeout (eg. in main.go) so that this logic can fire when we get the interrupt signal? WDYT?

Yeah, things could get out of hand if these delayed contexts exist all over the place, I agree.

But I'm not sure cleanup logic is the right concept either. For one, we would generally want cleanup logic related to a particular CR object to run as part of reconcile for better recovery mechanics. But also, in the case of an interrupted helm release, there is no safe cleanup after the helm release has been interrupted unless there is some signal from the chart author, the user, or both.

So I'm trying to think through a mechanic where:

1. we stop pulling items off the controller queues
2. we allow any in-flight reconciles to continue for some period of time. it would be interesting if we had some metrics history for a histogram of reconcile duration. I know we have a metric for that, but I don't think we have any historical data.

Also, I'm not at all attached to this PR, and I can definitely see how it has downsides. Another downside is that it could make issues due to interruptions even harder to find and reproduce.

This PR is mainly a lever for getting a conversation going, so I'm interesting to see where this leads!

Also of note, I just looked at our helm usage, and tracing through helm applier -> helm-operator-plugins -> helm, we are using the helm install/upgrade variants that use context.Background(), so I suspect that existing code is running up to terminationGracePeriodSeconds after the stop signal is received by the process.

Maybe we should switch over to Helm's RunWithContext methods in helm-operator-plugins. But that would risk making things worse if we don't have some other mitigation in place.

PR needs rebase.

I think it might be a good start to go through all of the reconciliation logic and try to identify places where an interruption could mean entering into this intermediate state. Once we have that we should better understand the scope of this and think about what can we do in each specific case to potentially mitigate/cleanup any sideffects to a working state or if there is anything we could actually do. Also, fully agree about taking into account any metrics we might have, or adding new ones to have a better understanding if need be.

Just thinking out loud, but one rough idea could be to keep track of how many reconciliations are in those critical places and communicate this outside of reconciliation loop and specific controller, so that upon receiving the interruption signal we could for example adjust the behavior/timeout on a more "global" level, which should be potentially easier to reason about and debug.

Update:
so to have something done right now, we could technically:

• pass a separate context to the one we get from ctrl.SetupSignalHandler() to manager
• catch an interruption via ctx := ctrl.SetupSignalHandler() like we currently do, but then allow a custom grace period timeout before we cancel the actual separate context we passed to manager

The main issue here is that the context we pass to manager at the start governs all of its operations including lifecycle of its controllers so I'm not sure what options would we have to handle stopping new reconciliation events in this scenario (or generally tbh) outside of this context. From what I've seen there are options like Unmanaged controllers, which are not automatically added to manager or more granular context separation, though that might lead to potentially similar issues with complexity and decreased debuggability.